Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate base/ platform from VS Code and adopt scroll bar #5096

Merged
merged 40 commits into from
Jul 11, 2024

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jul 9, 2024

Last one I swear 🤞

This PR includes the base/ platform directory from VS Code and replaces our old Viewport with DomScrollableElement. This makes the scroll bar much nicer, not only in term of UX but also to the developer as it's much more reliable and flexible. The new project vs/ includes a subset of base/ that includes only files that are referenced and then has some files changed that weren't getting tree shaken correctly (eg. the grapheme related stuff).

Included are 2 important scripts that will help us update base/ when needed in the future to pull in bug fixes and/or new features:

  • bin/vs_base_update.ps1 will clone the repo, copy files over and do some clean up
  • bin/vs_base_find_unused.js uses the typescript compiler API to find any unused files for deletion before doing a commit

@jerch finishing up for the day but it would be great if you could give this a review now, in particular trying out the new scroll bar and look at how src/vs/ is integrated (don't bother reviewing src/vs/base/ as it's already had a lot of eyes on it). I think the only thing left to do on my end is to clean up the new Viewport usage and fix the TODOs. Planning on merging tomorrow if all goes well.

TODO:

  • Resolve todos
    • Reimplement mouse events
  • Look into annoying ctrl+c issue where it doesn't go back to the bottom (not a regression)
  • Review Viewport implementation
  • Get CI passing
  • Record old/new core bundle sizes

@jerch
Copy link
Member

jerch commented Jul 10, 2024

@Tyriar Yupp most likely will be able to have a look at it in the afternoon. No clue yet if I manage to go deeper into details, ~20k loc is still a frightening number for a single quick review. I also have no doubt about code quality of the pulled in source, thus will skip most inner parts and mostly stick to a better conceptual comprehension.

One thing upfront - what you write about getting code fixes carried over with the help of additional scripts sounds a bit bothersome to me from a maintenance perspective. Do you see a chance, that your team would bundle&release the custom scrollbar functionality as a standalone package in the future? (Well idk if thats feasible or worth the trouble at all, have not yet looked at the code...)

@Tyriar
Copy link
Member Author

Tyriar commented Jul 10, 2024

One thing upfront - what you write about getting code fixes carried over with the help of additional scripts sounds a bit bothersome to me from a maintenance perspective.

I will probably be the one doing this and I doubt it will happen very often, like so infrequently that it definitely isn't worth trying to ship some lib. For example we will likely want to move over to the event and lifecycle code since it's more robust than ours and duplicates its functionality, and I think since those files are already present they won't need to bring in any changes.

Comment on lines +68 to +72
this.register(coreMouseService.onProtocolChange(type => {
this._scrollableElement.updateOptions({
handleMouseWheel: !(type & CoreMouseEventType.WHEEL)
});
}));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerch this was easy and works great 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, C-ish will safe the world someday, but before that, drive all programmers mad 😺

@tisilent
Copy link
Contributor

Prelude to the feasibility of GPU rendering in code editor. 😃

@Tyriar
Copy link
Member Author

Tyriar commented Jul 11, 2024

Here's the output, there's a pretty significant increase for the main xterm.js package but I think we should accept it still because:

  • This will become the legacy package and maybe soon be replaced by a more efficient esbuild-based one.
  • We will be able to remove parts of xterm.js in favor of base/ parts like events.ts and lifecycle.ts.
  • For VS Code specifically the terminal startup is also deferred shortly to ensure we don't block the time to get an editor up and running so the impact there should be minimal.

All the addons are the same as expected so nothing leaked in there.

.js output (webpack) - measured by looking at Windows Explorer (seems to be larger? kB vs KiB?):

File Before After
xterm.js 285kb 475kb (+67%)
xterm-addon-attach.js 2kb 2kb
xterm-addon-canvas.js 93kb 93kb
xterm-addon-clipboard.js 7kb 7kb
xterm-addon-fit.js 2kb 2kb
xterm-addon-image.js 55kb 55kb
xterm-addon-ligatures.js 193kb 193kb
xterm-addon-search.js 12kb 12kb
xterm-addon-serialize.js 16kb 16kb
xterm-addon-unicode11.js 12kb 12kb
xterm-addon-unicode-graphemes.js 18kb 18kb
xterm-addon-web-links.js 4kb 4kb
xterm-addon-webgl.js 99kb 99kb

.mjs output (esbuild) - measured by esbuild output:

File Before After
xterm.mjs 252kb 333kb (+32%)
xterm-addon-attach.mjs 2kb 2kb
xterm-addon-canvas.mjs 82kb 82kb
xterm-addon-clipboard.mjs 5kb 5kb
xterm-addon-fit.mjs 2kb 2kb
xterm-addon-image.mjs 52kb 52kb
xterm-addon-ligatures.mjs 199kb 199kb
xterm-addon-search.mjs 11kb 11kb
xterm-addon-serialize.mjs 16kb 16kb
xterm-addon-unicode11.mjs 11kb 12kb
xterm-addon-unicode-graphemes.mjs 14kb 14kb
xterm-addon-web-links.mjs 3kb 3kb
xterm-addon-webgl.mjs 87kb 87kb

@Tyriar
Copy link
Member Author

Tyriar commented Jul 11, 2024

Planning on merging this soon today. I want to look briefly at the ctrl+c/smooth scroll issue that's been driving me crazy for months.

@Tyriar Tyriar enabled auto-merge July 11, 2024 16:36
@Tyriar Tyriar merged commit f3bfe80 into xtermjs:master Jul 11, 2024
12 checks passed
@Tyriar
Copy link
Member Author

Tyriar commented Jul 11, 2024

Fixed microsoft/vscode#211199 finally! It was very difficult with the old viewport because of the mess of circular dependencies that was the scroll events

Tyriar added a commit to microsoft/vscode that referenced this pull request Jul 11, 2024
@Tyriar Tyriar added the breaking-change Breaks API and requires a major version bump label Jul 11, 2024
@Tyriar
Copy link
Member Author

Tyriar commented Jul 11, 2024

Maybe this will be a good opportunity to do a v6. While no big breaking changes it does:

  • Significantly change how the viewport works, so any custom styling of elements will break an embedder (like vscode)
  • fastScrollModifier now does nothing

@tisilent
Copy link
Contributor

lib/xterm.mjs 332.6kb 100.0%
├ src/common/InputHandler.ts 43.5kb 13.1%
├ src/browser/CoreBrowserTerminal.ts 21.0kb 6.3%
├ src/browser/services/SelectionService.ts 12.6kb 3.8%
├ src/browser/renderer/dom/DomRenderer.ts 11.2kb 3.4%
├ src/vs/base/browser/ui/scrollbar/scrollableElement.ts 10.3kb 3.1%
├ src/vs/base/common/event.ts 9.6kb 2.9%
├ src/common/parser/EscapeSequenceParser.ts 9.5kb 2.9%
├ src/browser/AccessibilityManager.ts 8.2kb 2.5%
├ src/common/buffer/Buffer.ts 7.4kb 2.2%
├ src/browser/Linkifier.ts 6.6kb 2.0%
├ src/vs/base/common/scrollable.ts 5.8kb 1.7%
├ src/common/buffer/BufferLine.ts 5.6kb 1.7%
├ src/browser/renderer/dom/DomRendererRowFactory.ts 5.4kb 1.6%
├ src/vs/base/browser/dom.ts 5.3kb 1.6%
├ src/common/CoreTerminal.ts 4.9kb 1.5%
├ src/browser/services/RenderService.ts 4.9kb 1.5%
├ src/browser/decorations/OverviewRulerRenderer.ts 4.8kb 1.4%
├ src/vs/base/browser/touch.ts 4.7kb 1.4%
├ src/common/Color.ts 4.5kb 1.4%
├ src/browser/public/Terminal.ts 4.5kb 1.3%
├ src/vs/base/browser/ui/scrollbar/abstractScrollbar.ts 4.4kb 1.3%
├ src/vs/base/common/lifecycle.ts 4.2kb 1.3%
├ src/vs/base/browser/fastDomNode.ts 4.1kb 1.2%
├ src/browser/services/ThemeService.ts 4.0kb 1.2%
├ src/browser/Viewport.ts 3.9kb 1.2%
├ src/common/input/Keyboard.ts 3.8kb 1.1%
├ src/vs/base/common/async.ts 3.5kb 1.0%
├ src/common/buffer/AttributeData.ts 3.4kb 1.0%
├ src/browser/decorations/BufferDecorationRenderer.ts 3.4kb 1.0%
├ src/vs/base/common/hash.ts 3.2kb 1.0%
├ src/browser/input/CompositionHelper.ts 3.0kb 0.9%
├ src/common/services/OptionsService.ts 2.9kb 0.9%
├ src/common/services/CoreMouseService.ts 2.7kb 0.8%
├ src/vs/base/browser/ui/scrollbar/scrollbarState.ts 2.7kb 0.8%
├ src/common/SortedList.ts 2.7kb 0.8%
├ src/common/CircularList.ts 2.6kb 0.8%
├ src/common/input/UnicodeV6.ts 2.5kb 0.8%
├ src/browser/services/CharacterJoinerService.ts 2.5kb 0.7%
├ src/common/input/TextDecoder.ts 2.4kb 0.7%
├ src/common/parser/Params.ts 2.4kb 0.7%
├ src/common/parser/OscParser.ts 2.4kb 0.7%
├ src/browser/services/CoreBrowserService.ts 2.3kb 0.7%
├ src/common/parser/DcsParser.ts 2.2kb 0.7%
├ src/browser/services/CharSizeService.ts 2.2kb 0.7%
├ src/vs/base/common/platform.ts 2.1kb 0.6%
├ src/vs/base/browser/mouseEvent.ts 2.0kb 0.6%
├ src/browser/renderer/dom/WidthCache.ts 2.0kb 0.6%
├ src/common/services/DecorationService.ts 2.0kb 0.6%
├ src/common/input/WriteBuffer.ts 2.0kb 0.6%
├ src/vs/base/browser/browser.ts 1.7kb 0.5%
├ src/vs/base/browser/keyboardEvent.ts 1.7kb 0.5%
├ src/vs/base/browser/ui/scrollbar/scrollbarArrow.ts 1.7kb 0.5%
├ src/browser/selection/SelectionModel.ts 1.7kb 0.5%
├ src/browser/decorations/ColorZoneStore.ts 1.6kb 0.5%
├ src/vs/base/browser/ui/scrollbar/horizontalScrollbar.ts 1.5kb 0.5%
├ src/browser/input/MoveToCell.ts 1.5kb 0.5%
├ src/common/data/Charsets.ts 1.5kb 0.5%
├ src/common/buffer/BufferSet.ts 1.5kb 0.4%
├ src/vs/base/browser/ui/scrollbar/verticalScrollbar.ts 1.4kb 0.4%
├ src/common/services/BufferService.ts 1.4kb 0.4%
├ src/vs/base/common/linkedList.ts 1.4kb 0.4%
├ src/vs/base/common/iterator.ts 1.4kb 0.4%
├ src/common/buffer/BufferReflow.ts 1.4kb 0.4%
├ src/browser/renderer/shared/SelectionRenderModel.ts 1.3kb 0.4%
├ src/common/services/LogService.ts 1.3kb 0.4%
├ src/vs/base/common/errors.ts 1.3kb 0.4%
├ src/common/services/CoreService.ts 1.3kb 0.4%
├ src/common/TaskQueue.ts 1.2kb 0.4%
├ src/vs/base/browser/ui/scrollbar/scrollbarVisibilityController.ts 1.2kb 0.4%
├ src/browser/OscLinkProvider.ts 1.2kb 0.4%
├ src/common/services/UnicodeService.ts 1.2kb 0.4%
├ src/browser/RenderDebouncer.ts 1.2kb 0.4%
├ src/common/services/OscLinkService.ts 1.2kb 0.4%
├ src/vs/base/common/keybindings.ts 1.1kb 0.3%
├ src/browser/TimeBasedDebouncer.ts 1.1kb 0.3%
├ src/common/input/XParseColor.ts 968b 0.3%
├ src/vs/base/common/arrays.ts 942b 0.3%
├ src/common/buffer/CellData.ts 937b 0.3%
├ src/vs/base/common/cancellation.ts 917b 0.3%
├ src/common/services/InstantiationService.ts 867b 0.3%
├ src/vs/base/common/keyCodes.ts 846b 0.2%
├ src/common/data/EscapeSequences.ts 836b 0.2%
├ src/vs/base/browser/iframe.ts 817b 0.2%
├ src/vs/base/browser/globalPointerMoveMonitor.ts 815b 0.2%
├ src/browser/Clipboard.ts 785b 0.2%
├ src/browser/services/MouseService.ts 714b 0.2%
├ src/common/Lifecycle.ts 714b 0.2%
├ src/common/Platform.ts 711b 0.2%
├ src/common/public/BufferNamespaceApi.ts 708b 0.2%
├ src/vs/base/common/collections.ts 695b 0.2%
├ src/vs/base/browser/ui/widget.ts 651b 0.2%

@Tyriar
Copy link
Member Author

Tyriar commented Jul 12, 2024

@tisilent good to know 👍

@Tyriar Tyriar modified the milestones: 5.6.0, 6.0.0 Jul 14, 2024
@jerch
Copy link
Member

jerch commented Jul 15, 2024

@Tyriar Yay thx for getting this & the ESM stuff done! 🥳

While the impact on bundle size is quite steep, I couldn't care less, since we get some solid and well tested stuff in return...

Edit: Yes, I second the idea to do a new major release at this point. Even though the API did not change substantially, the changes overall are still quite a lot, and well - ESM alone would qualify for that from an outer perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaks API and requires a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants